feat(thread_aware_core): add the stable core crate for thread-aware state - #643
feat(thread_aware_core): add the stable core crate for thread-aware state#643martintmk wants to merge 61 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3a761577-6422-4fd1-a0f7-88807d7f6134
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3a761577-6422-4fd1-a0f7-88807d7f6134
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3a761577-6422-4fd1-a0f7-88807d7f6134
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #643 +/- ##
=======================================
Coverage 100.0% 100.0%
=======================================
Files 584 586 +2
Lines 62913 62994 +81
=======================================
+ Hits 62913 62994 +81
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 392f99fd-c9b0-40c7-aad7-3e87a132b16e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 392f99fd-c9b0-40c7-aad7-3e87a132b16e
✅ Version increments look sufficient
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 392f99fd-c9b0-40c7-aad7-3e87a132b16e
Make the stable core crate dependency- and feature-free, and remove external-type implementations and their benchmarks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 392f99fd-c9b0-40c7-aad7-3e87a132b16e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 392f99fd-c9b0-40c7-aad7-3e87a132b16e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 392f99fd-c9b0-40c7-aad7-3e87a132b16e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 392f99fd-c9b0-40c7-aad7-3e87a132b16e
Keep the core crate dependency-free while providing Path, PathBuf, and HashMap implementations behind an opt-in std feature. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 392f99fd-c9b0-40c7-aad7-3e87a132b16e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 392f99fd-c9b0-40c7-aad7-3e87a132b16e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 392f99fd-c9b0-40c7-aad7-3e87a132b16e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 392f99fd-c9b0-40c7-aad7-3e87a132b16e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 392f99fd-c9b0-40c7-aad7-3e87a132b16e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f9c8e6e-73c6-47c0-830e-2f674656eceb
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f9c8e6e-73c6-47c0-830e-2f674656eceb
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f9c8e6e-73c6-47c0-830e-2f674656eceb
`cargo mutants` replaced `<impl Hash for Owner>::hash` with `()` and no test noticed: the only hash assertion checked that two owners sharing an identity hash *equally*, which a no-op hash satisfies trivially. Assert the converse as well -- two distinct owners with the same thread count must hash apart -- which fails under exactly that mutation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e215e3bc-7b6e-4743-afb4-55779bebd24b
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
crates/thread_aware_core/src/thread.rs:89
Owner::newusesAtomicUsize::fetch_add, which wraps on overflow. That would violate the documented guarantee that every newOwneris unique by potentially reissuing an identity once the counter wraps (especially on 32-bit targets). Consider usingfetch_updatewithchecked_addto fail fast on exhaustion without mutating the counter into a wrapped state.
pub fn new(min_threads: usize) -> Self {
Self {
id: NEXT_OWNER.fetch_add(1, Ordering::Relaxed),
min_threads,
}
crates/thread_aware_core/src/thread_aware.rs:165
- This crate consistently marks
#[cfg(test)] mod testswith#[cfg_attr(coverage_nightly, coverage(off))]to keep test-only lines from counting against the 100% coverage gate (e.g.crates/tick/src/error.rs). This test module is missing that attribute.
#[cfg(test)]
mod tests {
Resolved a conflict in the workspace Cargo.toml: main cascaded thread_aware, thread_aware_macros and thread_aware_macros_impl to 0.11.0, while this branch adds thread_aware_core. Kept main's version bumps and retained the new thread_aware_core = 0.1.0 entry. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 698f459b-8a21-45d8-a46f-1b39e71df42a
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/thread_aware_core/src/thread_aware.rs:165
- Tests modules in this repo are consistently annotated with
#[cfg_attr(coverage_nightly, coverage(off))]so unit-test-only lines don’t count against the coverage gate. Thismod testsis missing that attribute (unlikethread.rs/impls.rsin the same crate).
#[cfg(test)]
mod tests {
Review flagged the single-element case of `impl_transfer_tuple!` as broken. It is not: the arm emits a literal comma after the head, so the 1-tuple expands to `let (A,) = self` and `impl<A,> ThreadAware for (A,)`, both of which are correct (a trailing comma is legal in a generic parameter list). The existing coverage could not show this, though. `test_tuples` uses `(42,)`, and `i32::relocate` is a no-op, so the assertion holds whether or not the element was ever visited. Add a test built on `Tracker`, whose `relocate` flips an observable flag, covering the 1-tuple, the 2-tuple and the 12-element maximum. Verified the test is load-bearing: reproducing the reported defect, by dropping that literal comma, makes the 1-tuple bind the whole tuple and call its own impl again, overflowing the stack. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 698f459b-8a21-45d8-a46f-1b39e71df42a
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/thread_aware_core/src/thread_aware.rs:165
- The unit-test module here is missing
#[cfg_attr(coverage_nightly, coverage(off))]. Other test modules in this new crate already apply it (e.g., thread.rs and impls.rs), and omitting it can cause test-only lines to count against the coverage gate.
#[cfg(test)]
mod tests {
…cessors Vec now relocates through `as_mut_slice`, matching the array impl instead of repeating the slice loop. VecDeque keeps its own loop: its buffer is split, so it has no single slice to delegate to. Cell and RefCell were byte-identical, so they collapse into one macro arm. The internal macros move off the old "transfer" vocabulary onto the public `ThreadAware`/`relocate` names, and the stale comment above the leaf macro is replaced with one that says what the macro emits. Adds `#[inline]` to the trivial cross-crate accessors on `Owner`, `NumaNode` and `Thread`, plus the `()` impl. These are non-generic, exported and sit on the relocate path, which is rule 1 in docs/performance.md. `Owner::new` keeps its atomic RMW uninlined as requested. `Owner::eq` is included as well: it is the id comparison relocation actually performs, so inlining its callers but not it would be half a change. `Hash for Owner` is generic and already an inlining candidate, so it falls under rule 2 and is left alone. Renames the four `test_`-prefixed tests to behaviour-style names, and renames the `NumaNode::new` parameter to `node`. The `no_std` `Eq`/`Hash` shape cannot be pinned by a unit test: `id` is gated on `any(test, feature = "std")`, so `cfg(test)` restores it and a test gated on `not(feature = "std")` still sees three fields. Measured directly -- `size_of::<Thread>()` is 32 under `--no-default-features --lib`. The new `tests/no_std_surface.rs` links the crate as an ordinary dependency, where the gate resolves on the feature alone, and pins the two-field shape there. The `Thread` docs now state that equality and hashing compare owner and NUMA node alone without `std`, and a comment at the derive records why the unit tests cannot cover it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 698f459b-8a21-45d8-a46f-1b39e71df42a
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/thread_aware_core/src/thread_aware.rs:165
- The repo consistently marks
#[cfg(test)] mod testsblocks with#[cfg_attr(coverage_nightly, coverage(off))]to keep non-executed test-only code from counting against the coverage gate (see e.g.crates/benchmarking/src/lib.rs:155-157). This tests module is missing that attribute, so it should be added for consistency with the workspace’s coverage setup.
#[cfg(test)]
mod tests {
…`Thread` `Owner` names one live runtime, so making duplicates effortless worked against what the type is for. It keeps `Clone`, and `Thread::owner` now returns `&Owner` rather than handing out a copy on every call. Call sites that reused an owner after moving it into `Thread::new` now clone explicitly, which is the cost the change is meant to make visible. Comparisons against the accessor take a reference, and the `hash_of` test helper borrows. `owner_copy_preserves_the_thread_count` becomes `owner_clone_preserves_the_thread_count` and now also asserts the clone keeps the identity, not just the count. An `assert_not_impl_any!(Owner: Copy)` pins the decision so it cannot be re-derived by accident. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 698f459b-8a21-45d8-a46f-1b39e71df42a
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
crates/thread_aware_core/src/thread_aware.rs:165
- The internal
#[cfg(test)] mod testsblock here is missing#[cfg_attr(coverage_nightly, coverage(off))], which this repo commonly uses to keep unit-test scaffolding from counting against the coverage gate (e.g.,thread.rsandimpls.rsin this crate, andtick/src/fmt/mod.rs). Adding the attribute keeps this file consistent with the established coverage setup.
#[cfg(test)]
mod tests {
| use std::collections::hash_map::DefaultHasher; | ||
| use std::hash::{Hash, Hasher}; | ||
|
|
||
| use static_assertions::assert_impl_all; | ||
| use thread_aware_core::{NumaNode, Owner, Thread}; |
| #[derive(Clone, Debug)] | ||
| pub struct Owner { | ||
| id: usize, | ||
| min_threads: usize, |
There was a problem hiding this comment.
how min_threads is supposed to be used?
considering that it doesn't participate in Hash and PartialEq implementations, it might be better to remove it.
alternative suggestion:
- add
OwnerIdtype that stores onlyid: usize - remove
PartialEq,Hashimpl blocks for owner
There was a problem hiding this comment.
min_threads does not participate in Owner equality as it is only a hint to downstream customers for cases where they are reserving capacity for collections holding thread state.
Each instance of Owner gets it's own unique identity and we only have a single constructor. Later, if more information is needed on owner we can add Owner::builder() APIs where more properties can be set.
OwnerId can be added later to Owner::id if necessary, but currently it's not needed. At most, our code is comparing equality of 2 owners, which is already implemented correctly.
There was a problem hiding this comment.
so what's the purpose of this field in Owner struct?
There was a problem hiding this comment.
It's to hold more information about owner that consuming side might need. At some point owner could gain more properties.
There was a problem hiding this comment.
The thing I don't like the most here is that Owner combines two concepts with different semantics: identity and runtime metadata. That's what creates those awkward comparison impl blocks.
At the same time, if the goal of this PR is stabilization of thread_aware, and you already suggesting breaking changes, we should add Builder type right away, and may constructor methods private. Moreover, unless there is a defendable use case for min_threads, it's better to remove it from the type completely. Adding new fields when we have builder is not a breaking change, while removing it is.
There was a problem hiding this comment.
The thing I don't like the most here is that Owner combines two concepts with different semantics: identity and runtime metadata. That's what creates those awkward comparison impl blocks.
But it indeed has both. You cannot create 2 owners with the same properties that are equal. Owner identity is generated, each new owner get's completely new identity so there is no point comparing individual properties such as min_threads. Even if we need minimal OwnerId in the future, equality contract will stay the same - only "owner id" determines the equality, not it's properties.
At the same time, if the goal of this PR is stabilization of thread_aware, and you already suggesting breaking changes, we should add Builder type right away, and may constructor methods private.
It's purely additive change, but I can see a value of adding Owner::builder() now with min_threads being set to 0 by default.
Moreover, unless there is a defendable use case for min_threads, it's better to remove it from the type completely. Adding new fields when we have builder is not a breaking change, while removing it is.
Ralf Biedert (@ralfbiedert) Our scenario for min_threads is allocating collections with initial capacity. For thread_aware::Arc we cannot pre-initialize the collection because at the point of Arc creation we don't know the coordinates. Current solution is to create underlying collection on demand:
Is that work the extra complexity? Could we just get away without exposing min_threads initially?
…0806-stabilize-thread-aware
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
crates/thread_aware_core/src/thread_aware.rs:165
- The
#[cfg(test)] mod testsblock is missing the standard#[cfg_attr(coverage_nightly, coverage(off))]guard used elsewhere in the workspace to prevent test-only code from counting against the 100% coverage gate.
#[cfg(test)]
mod tests {
| /// Without `std` the thread id is gone, so equality and hashing cover the owner and the | ||
| /// NUMA node alone. `Thread` cannot be constructed here, which is exactly why that | ||
| /// narrowing is unobservable in a real program, so pin the shape by size instead: anything | ||
| /// wider than the two remaining fields means the id came back. | ||
| #[cfg(not(feature = "std"))] | ||
| #[test] | ||
| fn without_std_a_thread_carries_no_thread_id() { | ||
| let align = align_of::<Thread>(); | ||
| let expected = (size_of::<Owner>() + size_of::<NumaNode>()).div_ceil(align) * align; | ||
|
|
||
| assert_eq!(size_of::<Thread>(), expected, "a no_std `Thread` is its owner and its NUMA node"); | ||
| } |
| //! | ||
| //! # Library authors: implementing the trait | ||
| //! | ||
| //! **Library and application authors** implement [`ThreadAware`], usually through the |
There was a problem hiding this comment.
🤖: Non-blocking — The documented derive still implements the incompatible thread_aware::ThreadAware, not this crate's borrowed-Thread trait.
Please mark derive support as pending until thread_aware adopts this crate, and regenerate the README.
| impl_thread_aware!(NumaNode); | ||
| #[cfg(any(test, feature = "std"))] | ||
| impl_thread_aware!(std::thread::ThreadId); | ||
| impl_thread_aware!(Thread); |
There was a problem hiding this comment.
🤖: Non-blocking — No-op implementations for Thread, Owner, NumaNode, and ThreadId let derived cached-coordinate fields retain their old location after relocation.
Please document that cached-coordinate fields require hand-written relocate rather than field forwarding.
| impl_thread_aware!(NumaNode); | ||
| #[cfg(any(test, feature = "std"))] | ||
| impl_thread_aware!(std::thread::ThreadId); | ||
| impl_thread_aware!(Thread); |
There was a problem hiding this comment.
🤖: Non-blocking — Omitting PhantomData blocks derived implementations for generic marker fields; orphan rules prevent the higher-level crate from adding the implementation later.
Please add the incumbent no-op implementation and boundary tests, or document the exclusion.
Summary
Adds
thread_aware_core, the stable vocabulary that thread-aware libraries share. Nothingelse in the workspace changes.
ThreadAwarenotifies a value that it has moved, viarelocate(source: Option<&Thread>, destination: &Thread). The call is advisory: a valuemust remain correct if it is never called, called twice, or called with a
Threadit hasnever seen. That single property is what lets the method be infallible and lets a runtime
call it opportunistically.
Threadrecords where a value runs —Owner(which runtime owns it), astd::thread::ThreadId, andNumaNode(the memory closest to that thread). Fields areprivate and read through accessors.
Owneridentity is assigned, not supplied: every new owner is unique, so two runtimesalive at once cannot collide. It also reports
min_threads, the smallest number of threadsits runtime runs, so a value can pre-size per-thread state before it has seen any of them.
NumaNodeis an opaqueu32newtype built with an inherentconst fn new(u32).dev-dependency. The
stdfeature is on by default and adds implementations forHashMap,PathandPathBuf. Withdefault-features = falsethe crate needs onlyallocandpointer-width atomics;
Threadthen loses its thread id and cannot be constructed, leavingOwnerandNumaNodereadable so ano_stdlibrary can still implement the trait.docs/DESIGN.mdrecords why the crate is split out, and the rules that letThreadgaincoordinates later without breaking callers or changing behaviour.
docs/STABILIZATION.mdrecords the stable boundary.Why
Ownerassigns its own identityCallers used to choose the number, and the documentation warned at length that a collision
was "worse than a slowdown" and that keeping owners distinct was the embedding application's
problem. Taking the identity from a process-wide counter removes the hazard outright rather
than documenting it. Equality and hashing use identity alone.
min_threadsis a floor, not an estimate.0means the runtime promises none and spawns ondemand, which falls through
HashMap::with_capacityas a no-op, so both cases share oneexpression at the call site. It was named
threads_hintfirst; in std,_hintalready meansa bound —
size_hintreturns a lower bound plus an optional upper, and overshooting is a bug— so "hint" would have contradicted the suffix while leaving
0a magic value.Why the ids take
newrather thanFrom<u16>A
From<integer>impl on a permanently stable type is a one-shot commitment. Every way ofevolving one is breaking, verified against rustc:
From<u16>withFrom<u32>u16(E0277)From<u32>besideFrom<u16>from(1)falls back toi32(E0277)From<i32>from(1)to the new implcargo semver-checks0.48 reports "no semver update required" for all three, so none of themwould be caught in review.
NumaNodeis thereforeu32-backed with an inherentnew. Theaccepted width already exceeds any node count real hardware reaches — Windows' own topology
APIs report NUMA nodes as
USHORT— and a wider or fallible constructor can still be addedlater under a different name without touching existing callers.
Provided implementations
Containers forward to what they hold; primitives and other types with nothing tied to a
thread get an empty implementation. Three cases are deliberately absent:
&str,&Path) — relocating through one would adapt something the valueonly borrows, and whoever owns it is relocated on its own account.
Cow— relocating a borrowed one has to clone it into owned storage first, which istoo much work to hide behind an advisory call. It can return later if a caller wants it.
Arcand sets — whether a shared allocation should split per thread depends on what itholds, and mutating a set element could change its hash and corrupt the container.
Naming
thread_aware_core::Threadshares a name withstd::thread::Threadbut is a different kindof thing: an immutable coordinate that owns no OS resource. Importing both unqualified is
E0252, and both expose an identicalid(&self) -> ThreadId. This is called out in the cratedocs and in a
# Relation to std::thread::Threadsection on the type. Raised deliberatelyhere in case reviewers would prefer a different name.
Scope
thread_awareis unchanged and still ships its ownAffinity-based API. Nothing dependson
thread_aware_coreyet; adoption is left for a later change. The diff againstmainisthe new crate plus its workspace wiring (
Cargo.toml,Cargo.lock,CHANGELOG.md,README.md) and nothing more.Validation
--all-features; 25 unit tests and 12 doctests with--no-default-features. One doctest isignored: it demonstrates#[derive(ThreadAware)], which lives inthread_aware, and compiling it here wouldrequire the dependency the example exists to argue against.
-D warnings --cfg docsrs, formatting,spelling, and the generated README all pass.
Send,Sync,Unpin,UnwindSafe,RefUnwindSafe) on all three public types, andassert_obj_safe!pins dyn-compatibility ofThreadAware. Both guard silent regressions: a coordinate added later that is notSyncwould strip that property with no signature change, and a defaulted-but-generic trait method
would keep every
implcompiling while breaking everyBox<dyn ThreadAware>.found real defects rather than wording nits: a counter width that would wrap and reissue a
live identity, a claim that
ThreadIdis the operating-system thread id (it explicitly isnot), an equality claim that was false under
no_std, and anEq/Hashtest that passedvacuously because it compared a value with its own copy.